Merged
Conversation
- Returns nil if not loaded from DyldCache - Constructs FullDyldCache using the URL without its extensions
- Implemented a computed property in MachOFile to retrieve the offset. - Added a similar property in DyldCacheRepresentable for consistency. - Updated ObjCMethodList to utilize the new property from MachOFile.
- Ensure offset method returns nil if layout.offset is not positive - Ensure type method returns nil if layout.type is not positive
- Simplifies the code by eliminating unnecessary functionality
- Implement the `layoutOffset(of:)` method using kayPath as the default implementation. - Introduce the `UnresolvedValue` structure to resolve rebase/bind
- Changed dependency versioning for MachOKit in Package.swift - Updated revision in Package.resolved to match the latest state
- Introduced ResolvedValue struct to encapsulate address and offset information, improving clarity in the resolution process. - Removed deprecated code related to cache handling and rebase resolution, ensuring cleaner and more maintainable code.
- Deleted `fullCache` and `cache` properties as they were redundant
- Changed dependency from branch 'main' to version '0.40.0' - Updated revision in Package.resolved to match the new version
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the Mach-O Objective-C section handling to support split dyld cache layouts. The key changes involve centralizing fixup resolution through a new abstraction layer and adapting file I/O operations to work across multiple cache files.
- Introduces
UnresolvedValueandResolvedValuetypes to represent pre- and post-fixup values - Refactors the
_FixupResolvableprotocol to use field-based resolution instead of offset-based - Updates MachOKit dependency from 0.34.0 to 0.40.0
Reviewed Changes
Copilot reviewed 31 out of 32 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| Sources/MachOObjCSection/Util/FixupResolvable.swift | Adds value types and refactors protocol to use field-based resolution with keyPath approach |
| Sources/MachOObjCSection/Protocol/Protocol/_ObjCProtocolLayoutProtocol.swift | Comments out unused size and flags fields |
| Sources/MachOObjCSection/Protocol/Protocol/ObjCProtocolProtocol.swift | Migrates to field-based resolution pattern using unresolvedValue/resolveRebase |
| Sources/MachOObjCSection/Protocol/Ivar/_ObjCIvarLayoutProtocol.swift | Comments out unused alignment and size fields |
| Sources/MachOObjCSection/Protocol/Ivar/ObjCIvarProtocol.swift | Adopts new resolution pattern for ivar fields |
| Sources/MachOObjCSection/Protocol/Class/_ObjCClassLayoutProtocol.swift | Comments out unused swiftClassFlags field |
| Sources/MachOObjCSection/Protocol/Class/RO/_ObjCClassRODataLayoutProtocol.swift | Comments out unused flags, instanceStart, and instanceSize fields |
| Sources/MachOObjCSection/Protocol/Class/RO/ObjCClassRODataProtocol.swift | Refactors all field access to use new resolution pattern |
| Sources/MachOObjCSection/Protocol/Class/ObjCClassProtocol.swift | Updates class reading methods to use field-based resolution |
| Sources/MachOObjCSection/Protocol/Category/ObjCCategoryProtocol.swift | Migrates category protocol handling to new resolution pattern |
| Sources/MachOObjCSection/Model/Protocol/ObjCProtocolList64.swift | Refactors protocol list iteration to use UnresolvedValue approach |
| Sources/MachOObjCSection/Model/Protocol/ObjCProtocolList32.swift | Applies same protocol list refactoring for 32-bit architecture |
| Sources/MachOObjCSection/Model/Protocol/ObjCProtocol64.swift | Changes layoutOffset method to return KeyPath instead |
| Sources/MachOObjCSection/Model/Protocol/ObjCProtocol32.swift | Applies same keyPath change for 32-bit protocol |
| Sources/MachOObjCSection/Model/Property/ObjCPropertyList.swift | Rewrites property list parsing to use resolution pattern |
| Sources/MachOObjCSection/Model/Property/ObjCProperty.swift | Adds UnresolvedProperty and ResolvedProperty types |
| Sources/MachOObjCSection/Model/Method/ObjCMethodList.swift | Refactors method list parsing for new file I/O approach |
| Sources/MachOObjCSection/Model/Ivar/ObjCIvar64.swift | Updates to return KeyPath and removes layout offset calculation |
| Sources/MachOObjCSection/Model/Ivar/ObjCIvar32.swift | Applies same keyPath changes for 32-bit ivar |
| Sources/MachOObjCSection/Model/Class/RO/ObjCClassROData64.swift | Simplifies keyPath switch to only include used fields |
| Sources/MachOObjCSection/Model/Class/RO/ObjCClassROData32.swift | Applies same simplification for 32-bit class RO data |
| Sources/MachOObjCSection/Model/Class/ObjCClass64.swift | Refactors classROData loading with new resolution pattern |
| Sources/MachOObjCSection/Model/Class/ObjCClass32.swift | Applies same classROData refactoring for 32-bit |
| Sources/MachOObjCSection/Model/Category/ObjCCategory64.swift | Simplifies keyPath method for category fields |
| Sources/MachOObjCSection/Model/Category/ObjCCategory32.swift | Applies same category keyPath changes for 32-bit |
| Sources/MachOObjCSection/MachOFile+ObjectiveC.swift | Adds _fileSliceForSection helper and updates section parsing |
| Sources/MachOObjCSection/Extension/MachOFile+.swift | Adds file handle resolution and rebase helper methods |
| Sources/MachOObjCSection/Extension/FullDyldCache+.swift | New file adding fileHandle and fileSegment support |
| Sources/MachOObjCSection/Extension/DyldCacheRepresentable+.swift | New file adding relativeMethodSelectorBaseAddressOffset property |
| Sources/MachOObjCSection/Extension/DyldCache+.swift | Removes unused mainCache property |
| Package.swift | Updates MachOKit dependency to version 0.40.0 |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 31 out of 32 changed files in this pull request and generated 13 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| #endif | ||
|
|
||
| extension _FixupResolvable { | ||
| #if false |
There was a problem hiding this comment.
Dead code block (lines 102-133) should be removed rather than commented out with #if false. If this code needs to be preserved for reference, consider documenting why in a comment or moving it to version control history.
| } | ||
| } | ||
|
|
||
| #if false |
There was a problem hiding this comment.
Dead code block (lines 193-207) should be removed rather than commented out with #if false. Consider removing this unused extension entirely.
Comment on lines
+39
to
+40
| // case size | ||
| // case flags |
There was a problem hiding this comment.
Commented-out enum cases should be removed if they are no longer needed. If they need to be preserved for future use, add a TODO comment explaining why.
Suggested change
| // case size | |
| // case flags |
| associatedtype ObjCProtocolList: ObjCProtocolListProtocol where ObjCProtocolList.ObjCProtocol == Self | ||
|
|
||
| var layout: Layout { get } | ||
| // var layout: Layout { get } |
There was a problem hiding this comment.
Commented-out protocol requirement should be removed if no longer needed. The layout property is already inherited from LayoutWrapper protocol.
Suggested change
| // var layout: Layout { get } |
Comment on lines
+25
to
+26
| // case alignment | ||
| // case size |
There was a problem hiding this comment.
Commented-out enum cases should be removed if they are no longer needed. If they need to be preserved for future use, add a TODO comment explaining why.
Suggested change
| // case alignment | |
| // case size |
| associatedtype ObjCProtocolRelativeListList: ObjCProtocolRelativeListListProtocol where ObjCProtocolRelativeListList.List == ObjCProtocolList | ||
|
|
||
| var layout: Layout { get } | ||
| // var layout: Layout { get } |
There was a problem hiding this comment.
Commented-out protocol requirement should be removed if no longer needed. The layout property is already inherited from LayoutWrapper protocol.
Suggested change
| // var layout: Layout { get } |
| associatedtype ClassRWData: LayoutWrapper, ObjCClassRWDataProtocol where ClassRWData.Layout.Pointer == Layout.Pointer, ClassRWData.ObjCClassROData == ClassROData | ||
|
|
||
| var layout: Layout { get } | ||
| // var layout: Layout { get } |
There was a problem hiding this comment.
Commented-out protocol requirement should be removed if no longer needed. The layout property is already inherited from LayoutWrapper protocol.
Suggested change
| // var layout: Layout { get } |
| typealias ObjCProtocolList = ObjCClass.ClassROData.ObjCProtocolList | ||
|
|
||
| var layout: Layout { get } | ||
| // var layout: Layout { get } |
There was a problem hiding this comment.
Commented-out protocol requirement should be removed if no longer needed. The layout property is already inherited from LayoutWrapper protocol.
Suggested change
| // var layout: Layout { get } |
Comment on lines
+47
to
+57
| case .isa: \.isa | ||
| case .mangledName: \.mangledName | ||
| case .protocols: \.protocols | ||
| case .instanceMethods: \.instanceMethods | ||
| case .classMethods: \.classMethods | ||
| case .optionalInstanceMethods: \.optionalInstanceMethods | ||
| case .optionalClassMethods: \.optionalClassMethods | ||
| case .instanceProperties: \.instanceProperties | ||
| case ._extendedMethodTypes: \._extendedMethodTypes | ||
| case ._demangledName: \._demangledName | ||
| case ._classProperties: \._classProperties |
There was a problem hiding this comment.
[nitpick] The switch statement should use return explicitly for each case rather than relying on implicit return. This improves clarity and follows Swift best practices for non-trivial switch statements.
Suggested change
| case .isa: \.isa | |
| case .mangledName: \.mangledName | |
| case .protocols: \.protocols | |
| case .instanceMethods: \.instanceMethods | |
| case .classMethods: \.classMethods | |
| case .optionalInstanceMethods: \.optionalInstanceMethods | |
| case .optionalClassMethods: \.optionalClassMethods | |
| case .instanceProperties: \.instanceProperties | |
| case ._extendedMethodTypes: \._extendedMethodTypes | |
| case ._demangledName: \._demangledName | |
| case ._classProperties: \._classProperties | |
| case .isa: return \.isa | |
| case .mangledName: return \.mangledName | |
| case .protocols: return \.protocols | |
| case .instanceMethods: return \.instanceMethods | |
| case .classMethods: return \.classMethods | |
| case .optionalInstanceMethods: return \.optionalInstanceMethods | |
| case .optionalClassMethods: return \.optionalClassMethods | |
| case .instanceProperties: return \.instanceProperties | |
| case ._extendedMethodTypes: return \._extendedMethodTypes | |
| case ._demangledName: return \._demangledName | |
| case ._classProperties: return \._classProperties |
|
|
||
| return .init( | ||
| address: unresolvedValue.value, | ||
| offset: fileOffset(of: unresolvedValue.value)! |
There was a problem hiding this comment.
Force unwrapping could crash if fileOffset returns nil. Add proper error handling or document why this is guaranteed to succeed.
Suggested change
| offset: fileOffset(of: unresolvedValue.value)! | |
| // If fileOffset returns nil, fallback to 0 to avoid crash. | |
| offset: fileOffset(of: unresolvedValue.value) ?? 0 |
- Updated ObjCClass32, ObjCClass64, and ObjCMethodList extensions - Improved code clarity and consistency
- Simplified offset handling in readString calls for better clarity
- Simplified offset calculation by removing unnecessary variables - Used guard statement for fileHandle and fileOffset retrieval
…machokit-objc-support into feature/support-split-cache-layout
p-x9
commented
Nov 7, 2025
Comment on lines
-31
to
-42
| extension DyldCache { | ||
| var mainCache: DyldCache? { | ||
| if url.lastPathComponent.contains(".") { | ||
| var url = url | ||
| url.deletePathExtension() | ||
| return try? .init(url: url) | ||
| } else { | ||
| return self | ||
| } | ||
| } | ||
| } | ||
|
|
Comment on lines
-26
to
-38
| var cache: DyldCache? { | ||
| guard isLoadedFromDyldCache else { return nil } | ||
| guard let cache = try? DyldCache(url: url) else { | ||
| return nil | ||
| } | ||
| if let mainCache = cache.mainCache { | ||
| return try? .init( | ||
| subcacheUrl: cache.url, | ||
| mainCacheHeader: mainCache.header | ||
| ) | ||
| } | ||
| return cache | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
In the iOS dyld shared cache, the DATA and LINKEDIT sections are separated into distinct subcaches.
Fixed to account for these cases when reading sections.
Additionally, parts that had not undergone rebase processing have been corrected to ensure they are processed correctly.